branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777#65818
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: request changes
For distinct primary and secondary package names, this change leaves the canonical entries as the sole owners and makes the aliases non-owning, which fixes the direct ClearServices() double deletion. One PR-scoped correctness regression and one missing proof remain:
- If the configured secondary package equals the primary package, the non-owning assignments overwrite the only owning entries, so the service and method statuses leak.
- The Cloud lifecycle test still clears the secondary package specifically to avoid the reported destructor crash, so no runtime test executes this ownership branch.
Critical checkpoint conclusions:
- Goal and behavior: the collision-free shutdown path is improved, but the equal-package configuration introduced in the inline finding violates the ownership contract.
- Scope and clarity: this is a focused branch-4.0 backport of the BRPC patch change; the method and service ownership changes are symmetric and no unrelated source change is included.
- Lifecycle and error handling: distinct-key
ClearServices()is safe under the new ownership flags. The equal-key overwrite is the remaining PR-scoped lifecycle defect. Removal/rollback stale aliases were examined but are unchanged from the base patch and were excluded from the inline set. - Configuration and compatibility: no wire, protobuf, storage, or persistence format changes apply. The unrestricted
secondary_package_namesetting exposes the equal-package edge. - Concurrency, performance, and observability: the changed ownership fields are consumed by deletion guards; no new race, lock, hot-path, or observability regression was substantiated.
- Persistence, transactions, data writes, and FE/BE propagation are not applicable to this one-file third-party ownership delta.
- Validation:
git diff --checkpasses. This runner has nothirdparty/installedtree orprotoc, and the review contract forbids builds, so local validation was static. At the final sweep, GitHub reported Cloud UT and both macOS third-party builds successful; the Linux third-party build was still running, BE UT reported failure, and several regressions were pending. Cloud UT does not exercise the changed branch because the test forces the option empty. - User focus: no additional focus was provided; the full PR was reviewed.
Review completion: the sole changed file and its BRPC/Cloud upstream and downstream paths were reviewed through three rounds. Every candidate was accepted, deduplicated, or dismissed with concrete base-vs-head evidence, and all normal and risk-focused agents returned NO_NEW_VALUABLE_FINDINGS for the final two-comment set on head c5074691e385773ad0206f2737584c92c7d730b6.
Pick brpc thirdparty patch from #65777 for branch-4.0.\n\nThis PR only changes thirdparty/patches/brpc-1.4.0-secondary-package-name.patch. It intentionally does not include the master-only thirdparty/build-thirdparty.sh change from #65777.\n\nThe fix makes secondary brpc package aliases non-owning to avoid double delete during brpc::Server shutdown.\n\nValidation:\n- git diff --check\n- bash thirdparty/download-thirdparty.sh brpc\n\nIssue: DORIS-27128